Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@nodes/src/nodes/db_clickhouse/IGlobal.py`:
- Around line 67-85: The _build_connection_url function currently only
URL-encodes params['password'], leaving params['user'] and params['database']
unescaped which can break the DSN when they contain reserved characters; update
_build_connection_url to quote_plus both params['user'] and params['database']
(in addition to params['password']) and use the quoted values in both the TLS
branch (return in the if params.get('tls') block) and the non-TLS return path so
the constructed clickhouse+native://{user}:{password}@{host}/{database} string
is always safe; apply the same change pattern to the corresponding
_build_connection_url implementations in db_postgres/IGlobal.py and
db_mysql/IGlobal.py for parity.

In `@nodes/src/nodes/db_clickhouse/services.json`:
- Line 50: The current "description" in services.json describes the node as an
insert-only sink; update the description string for the node (the "description"
property in nodes/src/nodes/db_clickhouse/services.json) to reflect its primary
role as a query/agent tool that translates questions to SQL and executes queries
(question→SQL execution) and exposes the agent tool surface methods get_data,
get_schema and get_sql, while also noting it can insert or map fields for
ingestion—make the wording UI-friendly and accurate so users understand both
querying and optional ingestion capabilities.

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@nodes/src/nodes/db_clickhouse/IGlobal.py`:
- Around line 73-78: The TLS default-port logic incorrectly treats IPv6 literals
as having a port because it checks for ':' in the host; update the check used
when params.get('tls') is true so it's bracket-aware: for hosts starting with
'[' (IPv6 literal) detect a port only if there's a ']' followed by ':' (e.g.,
'...[ ]:port'), otherwise for non-bracketed hosts keep the existing ':' check;
if no port is detected append ':9440' to host. Refer to the params.get('tls')
check and the host variable in IGlobal.py and implement the bracket-aware port
detection before adding ':9440'.
- Around line 87-91: The _max_validation_attempts function currently casts
config.get('max_attempts', 5) to int but does not enforce the documented 1–20
bounds; update _max_validation_attempts to parse the value (catching
ValueError/TypeError), then clamp the resulting int into the inclusive range
1..20 and return that clamped value (falling back to 5 on parse failure), so the
'max_attempts' config key cannot produce 0, negatives, or values >20.
- Around line 57-61: The config values for host, user, database, and table are
being .strip()'d without guarding against explicit nulls; update the code that
builds the connection dict (the block creating 'host', 'user', 'password',
'database', 'table' entries in IGlobal.py) to coerce None to the intended
default before calling .strip() — e.g., use a None-safe expression like
(config.get('host') or 'localhost').strip() and similarly for 'user',
'database', and 'table'; also ensure 'password' is coerced to '' when None (but
keep the comment about not stripping).

In `@nodes/src/nodes/db_clickhouse/services.json`:
- Around line 73-76: The manifest currently exposes a write path via the
"answers" ingestion lane in services.json (see the "lanes" object) which lets
the node mutate ClickHouse even when only clickhouse.allow_execute is used;
either remove the "answers" lane from the default manifest or add explicit
write/disposal guards (e.g., "allow_write" and "allow_drop" flags) and wire
those flags into the node's ingestion/mutation path (the code paths that handle
answers ingestion and auto-create/insert behaviour referenced in the same
service and the related entries at lines 158-179) so that write/drop operations
are blocked unless the new toggles are explicitly enabled. Ensure the chosen
approach is applied consistently to the other affected service entries mentioned
(158-179).

In `@nodes/test/db_clickhouse/test_clickhouse_dsn.py`:
- Around line 98-170: Tests are missing regression coverage for three failure
modes: explicit nulls for stripped fields, IPv6 host literals with TLS, and
out-of-range max_attempts; update the test suite to add cases exercising
g._connection_params when fields like user/password/database are None (ensure
they result in empty/stripped values rather than errors), add TLS parsing tests
for IPv6 hosts (e.g. host like "[::1]" and "[::1]:9000") to verify
_build_connection_url preserves brackets and port and appends ?secure=true when
tls is truthy, and add bounds tests for g._max_validation_attempts to assert
that values below the minimum and above the maximum (and None/non-int) fall back
to the allowed range (and are cast to int where appropriate) so implementation
regressions are caught.

---

Duplicate comments:
In `@nodes/src/nodes/db_clickhouse/IGlobal.py`:
- Around line 67-85: The DSN builder in _build_connection_url only URL-encodes
params['password'], leaving params['user'] and params['database'] able to break
the SQLAlchemy URL; URL-encode both the username and database (e.g., via
urllib.parse.quote_plus) before interpolating them into the returned
clickhouse+native:// string while preserving existing host/port and ?secure=true
behavior (i.e., replace direct uses of params["user"] and params["database"]
with their quoted equivalents alongside the already-quoted password).

In `@nodes/src/nodes/db_clickhouse/services.json`:
- Line 50: The current "description" string in services.json describes an
insert-only ClickHouse sink; update the "description" value to accurately
reflect this node's primary role as a question-to-SQL querying tool with tool
functions (e.g., SQL generation, query execution, and optional insertion),
mentioning that it translates natural-language questions into SQL, runs queries
against ClickHouse, returns results, and can optionally map fields for inserts;
locate and edit the "description" array entry in
nodes/src/nodes/db_clickhouse/services.json to replace the insert-focused copy
with the new concise summary.

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@nodes/src/nodes/db_clickhouse/IGlobal.py`:
- Around line 108-109: The _db_description method can return None when config
contains "db_description": null which violates its -> str contract; update
_db_description (the method named _db_description) to coerce null to an empty
string the same way other nullable config fields are handled: read
config['db_description'], if it is None or not a string return '' (or str(value)
if you prefer) and always return a str so callers relying on a string never get
None.

In `@nodes/src/nodes/db_clickhouse/README.md`:
- Around line 69-71: The README's Ingestion section is ambiguous about the
"answers" lane; update the text to explicitly state that the node does not
expose the ingestion/input "answers" lane (used for pipeline inserts) but still
supports the output "questions -> answers" query path; specifically edit the
sentence referencing "this node intentionally does not expose a pipeline
ingestion (`answers`) lane" to add a clarifying clause like "this only removes
the ingestion/input `answers` lane (used for inserts), not the `questions ->
answers` output lane used for querying" so readers understand there is no
contradiction.

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In `@nodes/src/nodes/db_clickhouse/IGlobal.py`:
- Around line 60-64: Create a small normalization helper in IGlobal.py (e.g.,
normalize_field(value, default)) that coerces non-None values to str, calls
.strip(), and returns the default when the stripped result is empty; then
replace the current inline expressions for 'host', 'user', 'database', and
'table' to use normalize_field(config.get('host'), 'localhost') /
normalize_field(config.get('user'), 'default') /
normalize_field(config.get('database'), 'default') /
normalize_field(config.get('table'), 'table'); leave 'password' as
(config.get('password') or '') without stripping (per the existing comment) so
whitespace-only passwords are preserved.